Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Fee estimate RPC #1659

Merged
merged 1 commit into from
Oct 22, 2019
Merged

feat: Fee estimate RPC #1659

merged 1 commit into from
Oct 22, 2019

Conversation

jjyr
Copy link
Contributor

@jjyr jjyr commented Sep 28, 2019 •
edited
Loading

This PR adds a new RPC estimate_fee_rate. It takes the basic idea from bitcoin's estimatesmartfee, however, we ignore the magic numbers and tricks from the original code.

We estimate the tx fee rate by track txs that entered tx pool. See details https://github.com/nervosnetwork/ckb/pull/1659/files#diff-ff03764a87b23e747dadc645fcf8df8bR21

@jjyr jjyr mentioned this pull request Sep 28, 2019
Closed
@jjyr jjyr force-pushed the fee-estimate-rebase branch 2 times, most recently from f6fa5ae to 82bff49 Compare October 10, 2019 08:01
@jjyr jjyr marked this pull request as ready for review October 10, 2019 08:04
@jjyr jjyr requested a review from a team October 10, 2019 08:04
@jjyr jjyr force-pushed the fee-estimate-rebase branch from 82bff49 to d356fbe Compare October 10, 2019 08:15
quake
quake requested changes Oct 14, 2019
View reviewed changes
Copy link
Member

@quake quake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need some integration tests for estimate_fee_rate rpc

@jjyr
Copy link
Contributor Author

jjyr commented Oct 14, 2019

Need some integration tests for estimate_fee_rate rpc

There is a basic test case in the rpc_test, what the other cases do you want?

quake
quake requested changes Oct 14, 2019
View reviewed changes
tx-pool/src/process/estimator_process_block.rs Show resolved Hide resolved
tx-pool/src/service.rs Show resolved Hide resolved
util/fee-estimator/src/estimator.rs Show resolved Hide resolved
util/fee-estimator/src/tx_confirm_stat.rs Outdated Show resolved Hide resolved
util/fee-estimator/src/tx_confirm_stat.rs Show resolved Hide resolved
@quake
Copy link
Member

quake commented Oct 14, 2019 •
edited
Loading

Need some integration tests for estimate_fee_rate rpc

There is a basic test case in the rpc_test, what the other cases do you want?

I would like to have an integration test like this:
https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_fee_estimation.py#L127

Generate multiple txs through send_transaction rpc, mine them in blocks, then get fee estimation through estimate_fee_rate rpc.

@quake
Copy link
Member

quake commented Oct 14, 2019

One more question: the Estimator#track_tx method should be invoked when a tx entered pool, but I can't find related code in the tx-pool module, could you double check it?

@jjyr jjyr force-pushed the fee-estimate-rebase branch from d356fbe to ab3cd05 Compare October 15, 2019 10:52
@jjyr
Copy link
Contributor Author

jjyr commented Oct 15, 2019

One more question: the Estimator#track_tx method should be invoked when a tx entered pool, but I can't find related code in the tx-pool module, could you double check it?

Fixed, It's an error during rebase.

@jjyr jjyr requested a review from quake October 15, 2019 10:53
@jjyr jjyr force-pushed the fee-estimate-rebase branch 3 times, most recently from 6a5f781 to 29f3962 Compare October 16, 2019 02:17
@jjyr jjyr requested a review from a team October 16, 2019 03:52
@jjyr jjyr force-pushed the fee-estimate-rebase branch from 29f3962 to 72fb84e Compare October 16, 2019 06:22
@jjyr jjyr added the s:waiting-on-reviewers Status: Waiting for Review label Oct 16, 2019
quake
quake previously approved these changes Oct 17, 2019
View reviewed changes
@doitian doitian added this to the v0.24.0 milestone Oct 18, 2019
zhangsoledad
zhangsoledad previously approved these changes Oct 22, 2019
View reviewed changes
@jjyr jjyr dismissed stale reviews from zhangsoledad and quake via 89252b8 October 22, 2019 08:44
@jjyr jjyr force-pushed the fee-estimate-rebase branch from 72fb84e to 89252b8 Compare October 22, 2019 08:44
@jjyr
Copy link
Contributor Author

jjyr commented Oct 22, 2019 •
edited
Loading

Rebase due to Cargo.lock conflict, please re-approve @quake @zhangsoledad

@jjyr jjyr requested review from quake and zhangsoledad October 22, 2019 08:44
@jjyr
Copy link
Contributor Author

jjyr commented Oct 22, 2019

bors r=quake, zhangsoledad

@nervos-bot nervos-bot bot added the s:ready-to-merge Status: Waiting to be merged. label Oct 22, 2019
bors bot added a commit that referenced this pull request Oct 22, 2019
@bors @jjyr
Merge #1659
6fd895f 
1659: feat: Fee estimate RPC r=quake,zhangsoledad a=jjyr

This PR adds a new RPC [estimate_fee_rate](https://github.com/nervosnetwork/ckb/pull/1659/files#diff-622e6d119ac5d43f7eb41cb596159f9fR907). It takes the basic idea from bitcoin's [estimatesmartfee](https://bitcoincore.org/en/doc/0.16.0/rpc/util/estimatesmartfee/), however, we ignore the magic numbers and tricks from the original code.

We estimate the tx fee rate by track txs that entered tx pool. See details https://github.com/nervosnetwork/ckb/pull/1659/files#diff-ff03764a87b23e747dadc645fcf8df8bR21

Co-authored-by: jjy <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 22, 2019

Build failed

  • continuous-integration/travis-ci/push

@jjyr
Copy link
Contributor Author

jjyr commented Oct 22, 2019

bors r=quake, zhangsoledad

1 similar comment
@jjyr
Copy link
Contributor Author

jjyr commented Oct 22, 2019

bors r=quake, zhangsoledad

@bors
Copy link
Contributor

bors bot commented Oct 22, 2019

Already running a review

bors bot added a commit that referenced this pull request Oct 22, 2019
@bors @jjyr
Merge #1659
9ba3b87 
1659: feat: Fee estimate RPC r=quake,zhangsoledad a=jjyr

This PR adds a new RPC [estimate_fee_rate](https://github.com/nervosnetwork/ckb/pull/1659/files#diff-622e6d119ac5d43f7eb41cb596159f9fR907). It takes the basic idea from bitcoin's [estimatesmartfee](https://bitcoincore.org/en/doc/0.16.0/rpc/util/estimatesmartfee/), however, we ignore the magic numbers and tricks from the original code.

We estimate the tx fee rate by track txs that entered tx pool. See details https://github.com/nervosnetwork/ckb/pull/1659/files#diff-ff03764a87b23e747dadc645fcf8df8bR21

Co-authored-by: jjy <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 22, 2019

Build succeeded

  • continuous-integration/travis-ci/push

@bors bors bot merged commit 89252b8 into nervosnetwork:develop Oct 22, 2019
@yangby-cryptape yangby-cryptape mentioned this pull request Jun 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhangsoledad zhangsoledad zhangsoledad approved these changes

@quake quake quake approved these changes

Assignees
No one assigned
Labels
s:ready-to-merge Status: Waiting to be merged. s:waiting-on-reviewers Status: Waiting for Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jjyr @quake @zhangsoledad @doitian